Skip to content

fix(auth): serialize status imports with logout - #759

Open
mldangelo-oai wants to merge 4 commits into
mainfrom
mdangelo/codex/auth-status-test-isolation
Open

fix(auth): serialize status imports with logout#759
mldangelo-oai wants to merge 4 commits into
mainfrom
mdangelo/codex/auth-status-test-isolation

Conversation

@mldangelo-oai

@mldangelo-oai mldangelo-oai commented Aug 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

A status check can import ambient Codex credentials after a concurrent logout has removed them. Use the existing credential-home lock to serialize status imports and logout so a completed logout cannot be undone by that import.

This follows #738, which is now merged, and also isolates its authentication fixtures and documents credential-import errors. The branch is based on main.

Changes

  • Hold the existing credential-home lock around CLI and SDK status imports, and around logout plus its marker update. Always release the lock; SDK operations retain cancellation support.
  • Cover both operation orders for CLI and SDK with deterministic concurrency tests that exercise the real lock.
  • Give affected status tests temporary Codex homes, remove inherited API-key overrides using the existing helper, and restore the unrelated Python interpreter lookup.
  • Cover unreadable ambient credentials and document the existing CLI exit code 2 and SDK promise rejection.

Testing

CI repair at f025376ce7e3f325df37c68094b0ee2a4d2a9717 against main at 3c44b607dc05639a0e307a50d6dad1f822aeac00:

  • Flush React effects in the patch-browser test helper before sending another key, preventing a stale selection handler from receiving confirmation after deselection.
  • All nine patch-browser tests passed with coverage enabled, including 104 assertions.
  • SDK typecheck, changed-file formatting, and git diff checks passed.
  • The full test suite was left to CI.

Earlier validation recorded for prior heads:

Merge resolution at a6a645eb1ea7daee4da60dc96c1972b96ce3f09c against main at 3c44b607dc05639a0e307a50d6dad1f822aeac00:

  • Retained both chmod and relative imports in api-credentials.test.ts, preserving the tests from both branches.
  • Focused authentication tests with Bun 1.3.14 and seed 12345: 43 passed, zero failures across api-credentials.test.ts, cli-authentication.test.ts, and auth-status-concurrency.test.ts, including all four concurrency cases.
  • Formatting checks for all seven PR files, the plugin bundle build, and git diff --check passed.
  • The full test suite was left to CI.

Earlier validation of 9ceac8fdb4f54aa422e08124cc54b62f04e180c4, before this merge:

  • Focused authentication tests: 51 passed, zero failures, including all four concurrency cases.
  • Full randomized package suite, seed 3312411185: 2,120 passed, 43 skipped, zero failures.
  • pnpm run types, pnpm run format, plugin bundle build, and git diff --check passed.
  • The four concurrency regressions failed before the locking fix and passed with it.
  • The restricted fixed-seed run recorded 2,117 passes, 44 skips, and two failures: a localhost socket error and a sandbox-denied ps call. The ps failure also reproduces on unchanged main; both cases passed on an isolated rerun with native process access.

Risk and rollout

Status imports and logout now wait for other operations holding the same credential-home lock, including scans. No public CLI flags, environment variables, defaults, or dependencies change. API-key precedence and the existing credential-home and logout-marker behavior are preserved.

Unreadable-file tests are skipped on Windows and when running as root, where the POSIX permission fixture cannot reliably deny access. Shared authentication and concurrency tests remain enabled across platforms.

Public disclosure review

  • No customer, partner, prospect, or user identities, data, or identifying details are included.
  • No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
  • I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.

@mldangelo-oai

mldangelo-oai commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

Please review head b05c3b96331b4baa2e27385b3cbc0572f46b7457 against base 8521302acfcfdd53c138e86a60f3406d4733f66d. This is the four-file test and documentation follow-up stacked on #738.

@mldangelo-oai

mldangelo-oai commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator Author

@codex security review

Please review head b05c3b96331b4baa2e27385b3cbc0572f46b7457 against base 8521302acfcfdd53c138e86a60f3406d4733f66d, focusing on isolation from real credential homes in the authentication tests.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-04T06:11:13.013802Z f025376 Manual request
🔒 Security Review ✅ Completed 2026-09-04T06:11:51.150579Z f025376 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: b05c3b9633

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@mldangelo-oai

mldangelo-oai commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator Author

Validation for b05c3b96331b4baa2e27385b3cbc0572f46b7457:

  • Full package suite with seed 12345: 2,066 passed, 39 skipped, zero failures.
  • Full randomized package suite with seed 1964644220: 2,066 passed, 39 skipped, zero failures.
  • Focused authentication tests, type checks, formatting, and the plugin bundle build passed.
  • Local integration with main at f714a6582ec0b7fe32fc06894a3eb9cdd768b48f passed 47 focused tests, the SDK type check, and changed-file formatting after retaining both CLI imports in the parent's existing conflict.

The first Node CI attempt failed in the unchanged CLI launcher > runs through an installed npm-style bin symlink test on Windows/Node 24: spawnSync returned a null child-process status. The same test passed on Windows/Node 22. Only failed jobs were retried, without changing the commit. The retry passed: all 28 Node CI jobs are green. Both container workflows also passed.

Codex code review completed on this head with no major issues. This PR remains a draft, and #738 remains open and unchanged.

@mldangelo-oai mldangelo-oai mentioned this pull request Aug 30, 2026
Merged
3 tasks
mldangelo-oai pushed a commit that referenced this pull request Aug 30, 2026
@Ultron09
fix(auth): make login status recognize existing Codex authentication (#…
774da25 
…738)

Recognize ambient Codex authentication when querying CLI or SDK login status on a fresh state directory.

Add regression coverage for fresh-state credential import and the existing logout marker check. Concurrent status/logout handling is tracked in #759.
@mldangelo-oai
mldangelo-oai force-pushed the mdangelo/codex/auth-status-test-isolation branch from b05c3b9 to 9ceac8f Compare August 30, 2026 19:47
@mldangelo-oai mldangelo-oai changed the title test(auth): isolate ambient credential status fixtures fix(auth): serialize status imports with logout Aug 30, 2026
@mldangelo-oai
mldangelo-oai changed the base branch from mdangelo/codex/pr-738-base to main August 30, 2026 19:47
@mldangelo-oai

mldangelo-oai commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

Please review head 9ceac8fdb4f54aa422e08124cc54b62f04e180c4 against main at 774da259f8c5a35b62355521b117c8cd7a9ba950. This follow-up to merged #738 now includes the credential-home locking fix for concurrent status and logout, deterministic CLI/SDK concurrency coverage, and the existing test-isolation and import-error documentation changes.

@github-actions github-actions Bot added the bug Something isn't working label Aug 30, 2026
@mldangelo-oai

mldangelo-oai commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator Author

@codex security review

Please review head 9ceac8fdb4f54aa422e08124cc54b62f04e180c4 against 774da259f8c5a35b62355521b117c8cd7a9ba950, focusing on concurrent status/logout behavior, credential-home lock lifecycle, cancellation, API-key precedence, and isolation from real credentials in tests.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 9ceac8fdb4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@mldangelo-oai
mldangelo-oai marked this pull request as ready for review August 30, 2026 19:50
@mldangelo-oai

mldangelo-oai commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator Author

Validation for head 9ceac8fdb4f54aa422e08124cc54b62f04e180c4 against main at 774da259f8c5a35b62355521b117c8cd7a9ba950:

  • Node CI passed all 30 jobs, including the required Linux, macOS, and Windows checks and installed-package coverage.
  • Codex code and security reviews completed on this head with no findings or unresolved review threads.
  • The full randomized local suite passed with seed 3312411185: 2,120 passed, 43 skipped, zero failures. Focused authentication tests passed all 51 cases; type checks, formatting, and the plugin bundle build also passed.
  • Local fixed-seed validation did not pass. The restricted run encountered a localhost socket error and a sandbox-denied ps call; both cases passed in isolation with native process access, and the ps restriction also reproduced on unchanged main. The full native rerun then recorded 2,101 passes, 43 skips, 19 timeout failures, and nine between-test errors. All four concurrency regressions passed in that run. The cause of the full-run timeouts has not been isolated.

#738 is merged. This PR is rebased onto main, includes the deferred locking fix, and is now ready for review rather than a draft. It has not been merged.

@mldangelo-oai

mldangelo-oai commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

Please review head a6a645eb1ea7daee4da60dc96c1972b96ce3f09c against main at 3c44b607dc05639a0e307a50d6dad1f822aeac00. This merge resolves the credential-test import conflict while retaining both chmod and relative and both branches' tests. The three focused authentication test files passed all 43 tests, including the four concurrency cases. Formatting, the plugin bundle build, and whitespace checks passed; the full suite is left to CI.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: a6a645eb1e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@mldangelo-oai

mldangelo-oai commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

Please review head f025376ce7e3f325df37c68094b0ee2a4d2a9717 against main at 3c44b607dc05639a0e307a50d6dad1f822aeac00. Flush React effects in the patch-browser test helper before sending another key, preventing a stale selection handler from receiving confirmation after deselection.

All nine patch-browser tests passed with coverage enabled, including 104 assertions. SDK typecheck, changed-file formatting, and git diff checks passed. The full suite is left to CI.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: f025376ce7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kmbroai kmbroai Awaiting requested review from kmbroai

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@mldangelo-oai